fix: gate public presentation serialization on display_on_site for media uploads - #577
Open
JpMaxMan wants to merge 9 commits into
Open
fix: gate public presentation serialization on display_on_site for media uploads#577JpMaxMan wants to merge 9 commits into
JpMaxMan wants to merge 9 commits into
Conversation
…dia uploads Public/anonymous callers to the events/published endpoints could pull full PresentationMediaUpload data -- including live public S3 URLs -- for draft (display_on_site=false) uploads via ?expand=media_uploads, since PresentationSerializer read the unfiltered media uploads collection. Reported externally: a third party's calendar-scraping agent recovered pre-event draft slide decks this way. getVisibleMediaUploads() now reuses the existing admin/editor privilege check to filter to display_on_site=true uploads for Public callers, at all three call sites in this file. AdminPresentationCSVSerializer (admin-only) is untouched.
📝 WalkthroughWalkthroughChangesPresentation media visibility
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Caller
participant PresentationSerializer
participant Cache
Caller->>PresentationSerializer: Request presentation serialization
PresentationSerializer->>Cache: Read request-shaped cached payload
Cache-->>PresentationSerializer: Return cached fields or cache miss
PresentationSerializer->>PresentationSerializer: Resolve visible media uploads
PresentationSerializer-->>Caller: Return caller-specific presentation data
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-577/ This page is automatically updated on each push to this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/ModelSerializers/Summit/Presentation/PresentationSerializer.php`:
- Line 153: Update PresentationSerializer cache-key generation to distinguish
public output from private/admin output resolved through
AdminPresentationSerializer, using a private serializer-specific key for private
data. Ensure cached relation IDs still pass through the appropriate visibility
rules before being returned, including the logic around getVisibleMediaUploads
and the related cache handling at the referenced later section.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d0d53ee-95bd-4518-867f-495fd7dd1e9b
📒 Files selected for processing (1)
app/ModelSerializers/Summit/Presentation/PresentationSerializer.php
smarcet
reviewed
Aug 4, 2026
smarcet
reviewed
Aug 4, 2026
smarcet
reviewed
Aug 4, 2026
smarcet
reviewed
Aug 4, 2026
…ice accounts getMediaUploadsSerializerType() only recognised isAdmin() and memberCanEdit(), so two callers that OAuth2SummitEventsApiController::getSerializerType() already treats as privileged fell through to Public and lost every media upload once the display_on_site filter landed: - summit admins (summit-front-end-administrators). The event grid requests media_uploads.display_on_site, so the operator could no longer see the upload whose checkbox is the only thing that would make it visible again. - the content-snapshot service account. pub-api reads media_uploads with a client_credentials token, which carries no user_id, so getCurrentUser() is null by construction; the snapshot emptied and dropbox-materializer, which does not filter on the flag itself, staged nothing for every session. Service accounts are gated on a dedicated scope rather than on ApplicationType_Service alone, which would have handed drafts to every service client. The scope is registered with no endpoint association on purpose: endpoint scopes are matched with array_intersect (any-of), so associating it would admit a token holding only this scope to that endpoint. It is read straight off the token and never consulted through endpoint_api_scopes. Rollout order: the scope has to exist in openstackid and be granted to the content-snapshot client, and be added to pub-api's CONTENT_SNAPSHOT_OAUTH2_SCOPES, before this ships - until then that client still resolves Public.
The file is under tests/ root and no CI job filter covers it (push.yml runs
tests/oauth2/, tests/Unit/*, tests/Repositories/), so it rotted unnoticed and
failed on any environment. Three separate causes:
- setUp() read SummitMediaFileType via findAll() before insertSummitTestData(),
which opens with DELETE FROM SummitMediaFileType. The entity it kept pointed
at a deleted row, so the flush died on the SummitMediaUploadType.TypeID FK.
- self::$default_media_file_type is not a usable substitute: it carries ".PDF",
while SummitMediaUploadType::isValidExtension() compares strtoupper($ext)
against explode('|', ...), so a leading dot can never match. The test builds
its own type declaring PNG, matching the png it uploads and the format the
seeder uses (JPG|JPEG|PNG).
- the fixture declared Swift public storage, and serializing public_url builds
a download strategy for it, which needs an authUrl that neither the local
container nor CI provides. Local needs no credentials and the assertion is
about public_url being serialized, not about the backend behind it.
Green and repeatable across consecutive runs.
… caller getMediaUploadsSerializerType() resolves per user and per OAuth scope, but the cache key is built from id + LastEditedUTC + expand + fields + relations and has no audience component. A payload built for a privileged caller could therefore be served verbatim to an unprivileged one within the 1200s TTL, handing out display_on_site=false uploads the display_on_site filter was added to withhold. Adding the serializer class to the key would not close it: a speaker on the presentation and a plain attendee both serialize through PresentationSerializer, and a service account with ReadAllPresentationMediaUploads and one without it both serialize through AdminPresentationSerializer. Each pair shares a class and disagrees on this field. So the field is never stored. Cache::put receives a copy with media_uploads removed, and a new private withMediaUploads() resolves it fresh on the way out of every path -- cache hit, cache miss, and the non-cached branch alike. It opens by unsetting the field, so a payload written before this change cannot leak one either. Request shape is preserved: an id list for relations=media_uploads, serialized objects for expand=media_uploads, expand winning when both are present. This also removes the three scattered copies of that expansion logic, which were the reason the cache-hit branch could drift from the others in the first place.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-577/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/PresentationMediaUploadsTests.php (1)
54-55: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider a wider or non-repeating suffix for the test fixture name.
rand(1, 100)produces only 100 possible values. If theSummitMediaFileTypename column enforces uniqueness, or if a prior test run left residual rows, this can collide and cause an intermittent test failure. The static analysis tool flags this as CWE-338, but that classification does not apply here — the value only names a local test fixture and does not protect any credential, token, or access-control decision. Use a wider random range or a non-repeating identifier to reduce flakiness.♻️ Suggested change
- $media_file_type->setName("PNG_".rand(1, 100)); + $media_file_type->setName("PNG_".uniqid());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/PresentationMediaUploadsTests.php` around lines 54 - 55, The rand(1, 100) call in the SummitMediaFileType setName method on the media_file_type object produces only 100 possible values, which risks collision and test flakiness if the name column enforces uniqueness or prior test runs leave residual data. Replace the narrow random range with a much wider range (such as rand(1, 1000000)) or use a non-repeating identifier like uniqid() or microtime(true) to ensure each test fixture gets a unique name.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/ModelSerializers/Summit/Presentation/PresentationSerializer.php`:
- Line 230: Update the cache-read flow in the serializer method containing
withMediaUploads() to decode Cache::get($key) once and invoke withMediaUploads()
only when the decoded value is an array; treat null or any unavailable value as
a cache miss and continue the existing miss path. Add a regression test that
removes the cache key between Cache::has() and retrieval.
---
Nitpick comments:
In `@tests/PresentationMediaUploadsTests.php`:
- Around line 54-55: The rand(1, 100) call in the SummitMediaFileType setName
method on the media_file_type object produces only 100 possible values, which
risks collision and test flakiness if the name column enforces uniqueness or
prior test runs leave residual data. Replace the narrow random range with a much
wider range (such as rand(1, 1000000)) or use a non-repeating identifier like
uniqid() or microtime(true) to ensure each test fixture gets a unique name.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a9fa052-b9e6-4e59-aeaf-7f1cd7c20ca8
📒 Files selected for processing (5)
app/ModelSerializers/Summit/Presentation/PresentationSerializer.phpapp/Security/SummitScopes.phpdatabase/migrations/config/Version20260804120000.phpdatabase/seeders/ApiScopesSeeder.phptests/PresentationMediaUploadsTests.php
…hp suffix The testsuite in phpunit.xml scans ./tests/ with the default suffix, which is Test.php, so a file ending in Tests.php is never collected. The class was reachable only by passing its path explicitly - `--filter PresentationMediaUploadsTests` answered "No tests executed!" - which is a large part of why it rotted unnoticed until the fixture repair two commits ago. The class is renamed alongside the file: autoload-dev maps Tests\ to tests/ via PSR-4, so the two have to agree. No call sites to update; nothing referenced the old name.
Six cases, one per branch, as asked for in the PR thread on this method: anonymous, plain attendee, speaker on the presentation, summit admin, service account holding ReadAllPresentationMediaUploads, and service account without it. They assert on the output of serialize() - the media upload ids the caller receives - rather than on the serializer-type string the method returns. What the change is about is who sees an unapproved upload; the type is the mechanism, and pinning it would tie the suite to the current implementation of a decision that could be reached another way. The relations=media_uploads shape gives a bare id list, so an assertion can name the exact uploads without dragging PresentationMediaUploadSerializer, storage backends and public_url generation into a unit test. Narrowing fields to id keeps the attribute-mapping loop off every other getter on the mock. Each case was checked against a broken implementation rather than assumed to bite: - dropping isSummitAdmin() from the member condition fails the summit admin case alone, which is the regression that blanked the admin event grid - gating service accounts on ApplicationType_Service without the scope check fails the without-scope case alone - removing the display_on_site filter from getVisibleMediaUploads() fails all three unprivileged cases The suite also gets a matrix entry, by path: no job runs the tests/ root, only its subdirectories, so both this file and the one renamed in the previous commit would otherwise run nowhere.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-577/ This page is automatically updated on each push to this PR. |
Cache::has() vouching for a key does not guarantee Cache::get() returns it: the entry can reach its TTL, be evicted under memory pressure, or be dropped by a flush in the window between the two calls. json_decode(null, true) is null, and since the previous commit that null reaches withMediaUploads(array $values, ...) and raises a TypeError, so the race now answers 500 where it used to answer a quietly wrong payload. Only the voteable-presentation endpoints pass use_cache, which is why this had not surfaced. One Cache::get(), and anything that does not decode to an array falls through to the normal build path - the race, an ordinary miss, and a truncated write all take the same route, which is the one that was always going to be correct. Two tests, each checked against a broken implementation rather than assumed to bite: - testUnavailableCachedValueIsTreatedAsAMiss reproduces the race and fails with exactly that TypeError against the previous code. - testCacheHitIsServedButMediaUploadsAreResolvedFresh covers the other side, because the first test passes just as well against a serializer that has stopped reading the cache at all. It also pins the guarantee from the previous commit: returning the cached $values without recomputing makes the stale draft upload in the stored payload reach a public caller, which is the leak this branch exists to close. It asserts on the payload rather than on how the cache was consulted, so it does not have to be rewritten if that read changes shape again.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-577/ This page is automatically updated on each push to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR closes a data-leak path where public/anonymous callers could retrieve draft (display_on_site=false) PresentationMediaUpload details (including public URLs) from published event endpoints by requesting expand=media_uploads. It does so by gating which uploads are serialized based on caller privilege and the display_on_site flag, and introduces a dedicated scope for trusted service accounts.
Changes:
- Filter presentation media uploads for public callers to only those with
display_on_site=true, while preserving full visibility for admins/summit admins/editors and scoped service accounts. - Add
ReadAllPresentationMediaUploadsscope (constant + seeder + migration) to allow trusted service accounts to access draft uploads. - Add/adjust tests and CI workflow selection to cover the new visibility behavior and the cache-hit/miss behavior around
media_uploads.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/PresentationMediaUploadsVisibilityTest.php | Adds unit coverage for media upload visibility across anonymous/member/admin/service-scope branches and cache behavior. |
| tests/PresentationMediaUploadsTest.php | Fixes test class naming and stabilizes media upload type setup for CI/runtime environments. |
| database/seeders/ApiScopesSeeder.php | Registers the new ReadAllPresentationMediaUploads scope in seeded API scopes. |
| database/migrations/config/Version20260804120000.php | Adds an idempotent migration to register the new scope (without endpoint association). |
| app/Security/SummitScopes.php | Defines the new scope constant. |
| app/ModelSerializers/Summit/Presentation/PresentationSerializer.php | Implements visible-upload filtering, service-scope privilege, and safer cache read/write behavior for media_uploads. |
| .github/workflows/push.yml | Ensures the new root-level test file is executed in the CI matrix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…encoding
Closes the audience gap the media-uploads work left behind. serialize() is
inherited, not overridden, by AdminPresentationSerializer and by the track-chair
and CSV serializers, so all of them write through this cache, and
getAttributeMappings() merges $array_mappings across the hierarchy - the stored
payload carries rank, selection_status, streaming_url, etherpad_link,
overflow_stream_key, chair scores and vote stats. The key named the presentation
and the request shape but never the serializer, so a public caller repeating an
admin's query params inside the 1200s TTL read the admin payload back verbatim.
GET /summits/{id}/presentations/voteable has no admin gate and resolves the
serializer per caller, which is what makes the sequence reachable.
The parts are also no longer joined on characters they contain. "_" separated
them while appearing inside media_uploads, extra_questions and selection_plan,
so distinct requests could render one key: expand=media_uploads&fields=x and
expand=media&fields=uploads_x both flattened to "..._media_uploads_x_". The
encoding is what fixes this, not the digest - hashing the old concatenation
preserves it exactly, which the test asserts. sha256 rather than md5 because
these parts come from the query string and a collision here means serving one
audience's payload to another.
fields and relations are sorted first. Both are consumed with in_array(), so
order cannot change the payload and two spellings of one request no longer cost
two entries. $expand is left alone on purpose: its relations are dispatched in
order, and the speakers and moderator cases both write $values['moderator']
while disagreeing about moderator_speaker_id, so normalising it could merge two
payloads that are allowed to differ. A test pins that decision.
The id and last_edited stay outside the digest so an update still busts every
entry a presentation has, and so an operator can scan or drop them by pattern.
No migration: the format change orphans existing entries, which age out on the
TTL and rebuild on demand.
The comment on the key carries the invariant this rests on - audience-dependent
data either appears in the key or stays out of the cache. static::class is
sufficient only while the remaining differences are class-determined, which is
true today because the mappings are static, getSerializerType() is constant per
class, and media_uploads is stripped before Cache::put.
Refs ClickUp 86bb6aem0.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-577/ This page is automatically updated on each push to this PR. |
withMediaUploads() asks for the type once per media upload, and getVisibleMediaUploads() asks again on top of that, so a presentation with ten uploads resolved it twelve times for one response. The method reads the auth context and then runs memberCanEdit(), which goes through the member's speaker and this presentation's speaker collection - work that cannot change between two calls on the same serializer for the same caller. Measured on a ten-upload presentation serialized with expand and relations both naming media_uploads: 12 executions of the body before, 1 after. The memo is instance-scoped, not request-scoped, and that distinction is the whole point: memberCanEdit() is answered against THIS presentation, so a caller can be a speaker on one and a stranger to the next, and a request-wide memo would hand every presentation in a list the first one's answer. SerializerRegistry builds a fresh serializer per object and none outlive the request, so per-instance is both correct and enough. Private rather than protected because TrackChairPresentationSerializer and AdminPresentationCSVSerializer override the method with a constant and have nothing to memo. Also corrects the docblock, which opened by claiming this method is kept aligned with OAuth2SummitEventsApiController::getSerializerType() and only qualified that three paragraphs later. It is deliberately narrower for service accounts, which need ReadAllPresentationMediaUploads here and nothing beyond the application type there; a reader who stopped at the first sentence would conclude the opposite. Both raised by Copilot on PR 577.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-577/ This page is automatically updated on each push to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated no new comments.
Suppressed comments (2)
tests/PresentationMediaUploadsVisibilityTest.php:253
- This comment says the stored payload has media_uploads absent, but the mocked cached JSON includes a media_uploads field. Update the comment to match what the test is actually exercising (dropping/recomputing a stale cached media_uploads value).
// A payload as it is stored: media_uploads is absent by construction, and the stale value
// is one no unprivileged caller may receive.
tests/PresentationMediaUploadsVisibilityTest.php:112
- buildServiceContext() seeds scopes with the literal string '%s/summits/read', which is not a real scope value and makes the fixture less representative of production. Use the actual SummitScopes::ReadSummitData constant as the baseline scope instead.
$scopes = ['%s/summits/read'];
if ($with_scope) $scopes[] = SummitScopes::ReadAllPresentationMediaUploads;
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/PresentationMediaUploadsVisibilityTest.php (1)
31-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the expanded-media branch.
Lines 139-142 test only
relations=['media_uploads']. This does not executeapp/ModelSerializers/Summit/Presentation/PresentationSerializer.phpLines 201-215. Add a publicexpand=media_uploadsregression test that asserts the draft upload is absent from expanded output.Also applies to: 129-142
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/PresentationMediaUploadsVisibilityTest.php` around lines 31 - 33, Add a new public test method to PresentationMediaUploadsVisibilityTest.php that exercises the expanded-media code path in PresentationSerializer.php lines 201-215 by using expand=media_uploads parameter instead of the relations=media_uploads shape currently tested in lines 139-142. The new test should execute the same visibility scenario but verify that draft uploads are absent from the expanded output to ensure the serializer correctly filters them in both the bare id list and expanded representations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/ModelSerializers/Summit/Presentation/PresentationSerializer.php`:
- Around line 275-285: The json_encode call within the hash operation can return
false if malformed UTF-8 is encountered, which PHP silently coerces to an empty
string before hashing, creating a cache collision vulnerability. Capture the
result of json_encode into a variable, validate that it is not false, and handle
the failure case by either rejecting the request, applying
JSON_INVALID_UTF8_SUBSTITUTE or similar safe encoding options to json_encode, or
skipping the cache key generation entirely. Only pass valid encoded data to the
hash function.
---
Nitpick comments:
In `@tests/PresentationMediaUploadsVisibilityTest.php`:
- Around line 31-33: Add a new public test method to
PresentationMediaUploadsVisibilityTest.php that exercises the expanded-media
code path in PresentationSerializer.php lines 201-215 by using
expand=media_uploads parameter instead of the relations=media_uploads shape
currently tested in lines 139-142. The new test should execute the same
visibility scenario but verify that draft uploads are absent from the expanded
output to ensure the serializer correctly filters them in both the bare id list
and expanded representations.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f40fdf2-0cd2-4118-9a0e-4f9dd9a64bdd
📒 Files selected for processing (5)
.github/workflows/push.ymlapp/ModelSerializers/Summit/Presentation/PresentationSerializer.phptests/PresentationMediaUploadsTest.phptests/PresentationMediaUploadsVisibilityTest.phptests/PresentationSerializerCacheKeyTest.php
Comment on lines
+275
to
+285
| hash | ||
| ( | ||
| 'sha256', | ||
| json_encode | ||
| ([ | ||
| 'serializer' => static::class, | ||
| 'expand' => $expand ?? "", | ||
| 'fields' => $cache_fields, | ||
| 'relations' => $cache_relations, | ||
| ]) | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
For PHP 8.3, what does json_encode() return for malformed UTF-8 by default, and how does hash('sha256', false) behave when strict types are not declared?
💡 Result:
In PHP 8.3, the behavior of json_encode and the hash function is as follows: json_encode and Malformed UTF-8: By default, if json_encode encounters a malformed UTF-8 character sequence, it fails and returns false [1][2]. The global error state will be set to JSON_ERROR_UTF8, which can be verified using json_last_error [2][3]. To alter this behavior, you can use flags such as JSON_INVALID_UTF8_IGNORE (which ignores the invalid characters) or JSON_INVALID_UTF8_SUBSTITUTE (which replaces them with the Unicode replacement character U+FFFD) [4][3]. Alternatively, using the JSON_THROW_ON_ERROR flag will cause the function to throw a JsonException instead of returning false [1][3]. hash with false and Strict Types: The hash function in PHP 8.3 has a signature of hash(string $algo, string $data, bool $binary = false, array $options = []): string [5][6]. When you call hash('sha256', false), PHP performs internal type juggling. Because the second argument is declared as a string in the function definition, the boolean false is cast to an empty string ("") [5][7]. Consequently, hash('sha256', false) behaves identically to hash('sha256', '') and returns the SHA-256 hash of an empty string, which is e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855. This behavior remains consistent regardless of whether strict types are declared using declare(strict_types=1;); because the hash function is a built-in PHP function, the engine will still attempt to coerce the input to the required string type rather than throwing a TypeError.
Citations:
- 1: https://stackoverflow.com/questions/70055942/json-encode-returns-false-when-i-pass-it-non-ascii-characters
- 2: https://www.php.net/manual/en/function.json-last-error.php
- 3: https://www.php.net/manual/en/json.constants.php
- 4: https://www.php.net/function.json-encode.php
- 5: https://www.php.net/function.hash.php
- 6: https://php.watch/codex/hash
- 7: https://durak.org/sean/pubs/software/php-8.3.0/function.hash.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="$(fd -a 'PresentationSerializer.php' . | head -n1 || true)"
echo "FILE=$file"
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '1,220p' "$file"
echo "--- 220-320 ---"
sed -n '220,320p' "$file"
fi
echo "--- declarations strict_types and call sites ---"
rg -n "declare\\(strict_types|function serialize|cacheFields|cacheRelations|cache\\(|hash\\(" .Repository: OpenStackweb/summit-api
Length of output: 41789
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="app/ModelSerializers/Summit/Presentation/PresentationSerializer.php"
echo "--- PresentationSerializer.php header and relevant sections ---"
sed -n '1,180p' "$file"
echo "--- 180-340 ---"
sed -n '180,340p' "$file"
echo "--- Trait RequestScope/cache methods ---"
sed -n '1,120p' app/ModelSerializers/Traits/RequestScopedCache.php
echo "--- SerializerRegistry signature around global serialization ---"
sed -n '760,830p' app/ModelSerializers/SerializerRegistry.php
echo "--- behavioral probe for PHP hash(json_encode failure unless unavailable ---"
if command -v php >/dev/null 2>&1; then
php -r '
$encoded = json_encode(["x" => "\xFF"]);
var_export($encoded);
echo "\n";
var_export(is_string($encoded));
echo "\n";
echo hash("sha256", $encoded);
echo "\n";
echo hash("sha256", false);
'
else
echo "php not available"
fiRepository: OpenStackweb/summit-api
Length of output: 21189
Handle failed cache-key encoding before hashing.
If malformed UTF-8 reaches serialize(), json_encode() returns false, and PHP coerces that value to an empty string before hash('sha256', ...). Reject the request, use invalid-UTF8-safe encoding options, or build the key without caching.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/ModelSerializers/Summit/Presentation/PresentationSerializer.php` around
lines 275 - 285, The json_encode call within the hash operation can return false
if malformed UTF-8 is encountered, which PHP silently coerces to an empty string
before hashing, creating a cache collision vulnerability. Capture the result of
json_encode into a variable, validate that it is not false, and handle the
failure case by either rejecting the request, applying
JSON_INVALID_UTF8_SUBSTITUTE or similar safe encoding options to json_encode, or
skipping the cache key generation entirely. Only pass valid encoded data to the
hash function.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/9014802374/86bb7zx8t
ref: https://app.clickup.com/t/9014802374/86bb6aem0
Summary
Public/anonymous callers to the events/published endpoints could pull full
PresentationMediaUploaddata — including live public S3 URLs — for draft(
display_on_site=false) uploads via?expand=media_uploads, becausePresentationSerializerread the unfiltered media uploads collection with novisibility check.
Reported externally: a third party's calendar/speaker-scraping AI agent recovered
pre-event draft slide decks this way, with no malicious intent — it just added
expand=media_uploadsto a normal public schedule call.Closing that hole exposed three further defects in the same serializer, all fixed
here: privileged callers were losing uploads they were entitled to, the response
cache had no audience component, and the cache read had a check-then-act race.
Root cause
GET /summits/{id}/events/publishedandGET /summits/{id}/events/{event_id}/publishedare fully public/unauthenticated and forward a caller-supplied
expandwith no allowlist.PresentationSerializer::serialize()handledexpand=media_uploads(and therelationsid-list path) by callingPresentation::getMediaUploads()— unfiltered —and serializing every attached upload's
public_urlregardless of draft state.display_on_siteexists onPresentationMaterial/PresentationMediaUploadfor exactlythis distinction (defaults
false), andSummitMediaUploadType::getMediaUploadsToDisplayOnSite()was even built to filter on it —but had zero callers in the API/serializer stack.
What changed
1. Public callers only see approved uploads (
90904aa91)getVisibleMediaUploads()filters todisplay_on_site=truewhenever the resolvedserializer type is Public.
AdminPresentationCSVSerializeris intentionally untouched.2. Privileged callers get their uploads back (
7455f6006)getMediaUploadsSerializerType()only recognisedisAdmin() || memberCanEdit(), so twocallers that
OAuth2SummitEventsApiController::getSerializerType()already treats asprivileged fell through to Public and lost every upload once the filter landed:
summit-front-end-administrators). The event grid requestsmedia_uploads.display_on_site, so the operator could no longer see the upload whosecheckbox is the only thing that would make it visible again — circular.
client_credentialstoken, which carries no
user_id, sogetCurrentUser()is null by construction.Service accounts are gated on a new scope,
ReadAllPresentationMediaUploads, ratherthan on
ApplicationType_Servicealone, which would have handed drafts to every serviceclient. The scope is registered with no endpoint association on purpose: endpoint
scopes are matched with
array_intersect(any-of), so associating it would admit a tokenholding only this scope to that endpoint.
3.
media_uploadsis never cached (b4079911e)Visibility is resolved per user and per scope, but the cache key has no audience
component, so a payload built for one caller could be served to another. Adding the
serializer class to the key does not fix this case — a speaker and a plain attendee both
use
PresentationSerializer, and a service account with and without the scope both useAdminPresentationSerializer. So the field is stripped beforeCache::putand recomputedon every return through a new
withMediaUploads(), on both the hit and miss paths.4. The cache read no longer races (
e09b148f3)Cache::has()followed byCache::get()returns null if the entry expires or is evictedbetween the two, and
json_decode(null, true)reachingwithMediaUploads(array $values)raised a
TypeError. OneCache::get(); anything that does not decode to an array fallsthrough and is rebuilt.
5. The cache key gained a serializer and an unambiguous encoding (
dea76db68) —closes ClickUp 86bb6aem0
serialize()is inherited, not overridden, byAdminPresentationSerializerand thetrack-chair and CSV serializers, and
getAttributeMappings()merges mappings across thehierarchy — so the stored payload carried
rank,selection_status,streaming_url,etherpad_link,overflow_stream_key, chair scores and vote stats. With no class in thekey, a public caller repeating an admin's query params inside the 1200s TTL read that
payload back verbatim.
The key is now
presentation_{id}_{lastEditedTs}_{sha256}, with the digest overstatic::class,expand,fieldsandrelations. The parts arejson_encoded ratherthan joined on
_, which also occurs insidemedia_uploads,extra_questionsandselection_plan—expand=media_uploads&fields=xandexpand=media&fields=uploads_xpreviously rendered the same key.
fieldsandrelationsare sorted first;expandisdeliberately not, because its relations are dispatched in order and the
speakersandmoderatorcases interact throughmoderator_speaker_id.6. Serializer-type resolution memoized (
23a45c253)It ran once per media upload plus once per
getVisibleMediaUploads()— 12 executions fora ten-upload presentation, measured — and it reaches
memberCanEdit(), which touches themember's speaker and this presentation's speaker collection. Memoized per serializer
instance, not per request:
memberCanEdit()is answered against this presentation, soa request-wide memo would hand every presentation in a list the first one's answer.
Deployment steps
service account resolves Public,
media_uploadscomes back empty inevents.jsonandpresentations.json, pub-api does not validate response shape soSnapshotCompletedstillfires, and dropbox-materializer stages nothing for every session — silently.
1. openstackid — register and grant the scope (outside this repo)
Register
{SCOPE_BASE_REALM}/summits/presentations/media-uploads/read/allon thesummit-api resource server and grant it to the content-snapshot client.
SCOPE_BASE_REALMisconfig('app.scope_base_realm')per environment, e.g.https://api.dev.fnopen.com/summits/presentations/media-uploads/read/allon dev.2. pub-api — request the scope (outside this repo)
Append the same value to
CONTENT_SNAPSHOT_OAUTH2_SCOPES(
backend/.env.template:22, read atbackend/settings.py:321; space-separated) andredeploy. Safe to do before summit-api ships — the current code simply ignores the extra
scope on the token.
3. summit-api — deploy this branch, then run the config migration
Version20260804120000registers the scope inapi_scopes. It is idempotent(
WHERE NOT EXISTS) and reversible viadown(). Noendpoint_api_scopesrow is created,by design — see above.
ApiScopesSeedercarries the same entry for fresh installs only.4. Cache — no action
The key format change orphans existing entries; they age out on the 1200s TTL and rebuild
on demand. No flush, no coordination.
5. Verify after deploy
?expand=media_uploadson a published event returns onlydisplay_on_site=trueuploads.media_uploads, and dropbox-materializer stagesfiles.
Test plan
Automated —
tests/PresentationMediaUploadsVisibilityTest.php,tests/PresentationSerializerCacheKeyTest.php,tests/PresentationMediaUploadsTest.php(13 tests). CI runs them via the new
PresentationMediaUploadsmatrix entry inpush.yml;the files sit in the
tests/root, which no existing job covered.display_on_site=trueuploadsdrafts too
fields/relationsorder reuses one entry;expandorder does notmedia_uploadsresolved freshEach automated test was checked against a deliberately broken implementation, so it is known
to fail when the behaviour regresses rather than assumed to.
OAuth2PresentationApiTestis unchanged frommain: 42 tests, 204 assertions, 6 failures,2 skipped. Those 6 are pre-existing (
Doctrine\ORM\EntityNotFoundExceptiononMember) anduntouched by this branch.
Out of scope / follow-up
public-read ACL already on previously-uploaded draft files. Rotating/regenerating
already-exposed files (
use_temporary_links_on_public_storage+ the existingpresentations-regenerate-media-uploads-temporal-public-urlscommand, or a longer-termprivate-storage migration) is a separate ops decision for @smarcet.
display_on_sitebackfill. Data audit: 707 rows at1, 4514 at0; every summit since63 is at exactly zero approved.
DocumentsComponent.js:23filters the same flag client-side,so event-site already renders zero media uploads for those shows — the files have always been
in the payload and never displayed. A blanket backfill would publish material deliberately
left unapproved on 12/31/63, and on the shows at zero the flag means "never triaged" rather
than "reviewed and rejected".
Summary by CodeRabbit